Add time navigation buttons by jgowdyelastic · Pull Request #8546 · elastic/kibana

jgowdyelastic · 2016-10-05T10:57:17Z

Adds zoom in/out and move left/right buttons to kibana's time picker.

Closes #1956

epixa · 2016-10-05T12:41:44Z

jenkins, test this

epixa · 2016-10-05T12:52:44Z

@jgowdyelastic Can you reissue this PR against master? We'll then backport it to 5.x.

jgowdyelastic · 2016-10-05T13:10:40Z

Done, rebased on master and PR is now against master.

epixa · 2016-10-05T13:12:02Z

@jgowdyelastic Thanks!

epixa · 2016-10-05T13:37:57Z

src/ui/public/timepicker/kbn_global_timepicker.js

These should all be const unless you will be overriding the reference, in which case they would be let.

ok, good point. i've pushed a change for this.
This was just copied from the kibana 4.x prelert app, which doesn't have much ES6 syntax.

LeeDr · 2016-10-06T11:27:13Z

Please add automated and/or manual tests for this new feature.

dimitris-athanasiou · 2016-10-06T14:14:50Z

Just letting you folks @jgowdyelastic is on vacation, back on Tuesday 11th. He is not ignoring you all.

thomasneirynck

Looks great! I like the addition. It is very useful addition I think.

I only have some real minor cosmetic things.

thomasneirynck · 2016-10-06T20:06:29Z

src/ui/public/timepicker/kbn_global_timepicker.js

this import is obsolete. linter was also complaining about unused function arguments in this file as well: config, newval, oldVal, $el, attrs

While we're at it, we may as well remove them ;)

thomasneirynck · 2016-10-06T20:09:36Z

src/ui/public/timepicker/kbn_global_timepicker.js

minor: consider reordering the to/from assignments for symmetry with all the others.

thomasneirynck · 2016-10-06T20:11:15Z

src/ui/public/timepicker/kbn_global_timepicker.js

minor: consider removing this comment, I think the code is nice & dense enough to be self explanatory.

thomasneirynck · 2016-10-06T20:16:35Z

src/ui/public/timepicker/kbn_global_timepicker.js

minor: consider not doing the side-effect in this getter/setter. If you prefer to avoid two function-calls, perhaps rename this function to something like setToTimeToAbsoluteAndGetToFrom (yeah, i know :$). But now, this state change is a little buried.

thomasneirynck · 2016-10-06T20:18:32Z

src/ui/public/timepicker/kbn_global_timepicker.html

minor: I wouldn't mind tooltips on these buttons actually. 'move [forward|backward] in time', 'zoom out timeline', 'zoom in timeline'

tooltips added

thomasneirynck · 2016-10-06T20:25:59Z

src/ui/public/timepicker/kbn_global_timepicker.html

sorry to hijack this PR for something a little broader ;)

With the 4 new additions, the top toolbar now looks like the above. From a UI perspective, would it make sense to better isolate the timepicker tools from the "new/save/open/share" buttons. Both are unrelated thematically. Putting a divisor in between/shading background differently/ .... might make the grouping a little clearer. @alt74, @cjcenizal, thoughts (?)

I think that, ultimately, we should rebuild the timepicker component so that these arrow buttons are part of the dropdown that appears when you click the time range. I don't think the user needs to have them visible at all times. However, this will take some time to implement, and I don't think we should hold up this PR for that.

I think the immediate problem is that the "back" arrow blends in with the + and - icons. We can create the separation you mention by moving them both to the right side, and letting the time range itself create visual separation:

As an additional benefit, the user doesn't need to move the mouse as far when navigating back and forward in succession.

What do you think?

I agree that the timepicker controls should be obviously separate from the rest of the menu.
I've added a separator as I think it's the simplest and less intrusive solution.

I prefer the arrow buttons being either side of the date range as it's obvious that you'll be moving either side of the displayed range.
But it doesn't make too much difference either way. So I'm happy to be overridden.

Ultimately I think these controls should remain in the navigation bar and not inside the dropdown. They provide a fast way of navigating through time and made a noticeable difference when we originally added them to the Prelert app.

I think this looks good. I'm not 100% confident about it but that's because I haven't used it as much as you have. Once I've had the chance to use it for awhile I'm sure I'll either be on board or be able to provide some better suggestions. :)

IMHO, the arrows work better on either side of the range, for the reasons mentioned by @jgowdyelastic

cjcenizal · 2016-10-10T19:18:30Z

src/ui/public/styles/base.less

This can just be .time-nav-btn because using the element selector increases the selector's specificity unnecessarily.

agreed. element selector removed

cjcenizal · 2016-10-10T19:50:58Z

src/ui/public/timepicker/kbn_global_timepicker.html

I think that, ultimately, we should rebuild the timepicker component so that these arrow buttons are part of the dropdown that appears when you click the time range. I don't think the user needs to have them visible at all times. However, this will take some time to implement, and I don't think we should hold up this PR for that.

I think the immediate problem is that the "back" arrow blends in with the + and - icons. We can create the separation you mention by moving them both to the right side, and letting the time range itself create visual separation:

As an additional benefit, the user doesn't need to move the mouse as far when navigating back and forward in succession.

What do you think?

tbragin · 2016-10-23T19:29:24Z

Could I ask for some screenshots or a quick animated gif in this PR? Will help reviewers that don't have time to check it out to at least have an idea where in the interface these new buttons are.

thomasneirynck · 2016-10-24T05:12:32Z

src/ui/public/styles/base.less

+1 for me, I think it looks good.

thomasneirynck · 2016-10-24T05:16:13Z

@tanya this was buried in one of those "outdated" collapsing sections, but here is a screenshot to what it looks like: #8546 (comment). The arrows move the time forward and backward with the size of the current interval, and the magnifiers zoom out and or zoom in the time with a factor of two

kimjoar · 2016-10-24T05:33:15Z

src/ui/public/timepicker/kbn_global_timepicker.js

typo, foward -> forward

ha, embarrassing....
i've corrected this, thanks

kimjoar · 2016-10-24T05:52:15Z

I find the icons confusing, but there are tooltips so it's probably good enough. However, I think we should add better descriptions to the tooltips. For example, I think Zoom in and Zoom out are unclear about what you're actually doing (Maybe something along the lines of Double the timespan — or maybe we already have some other word we use for the timepicker range(?)).

Also, to me something looks off about the spacing between all the timepicker controls. I think it needs tighter spacing between the controls, plus I think it should have the same amount of padding on each side of the new border.

ppisljar · 2016-10-24T08:42:24Z

this looks really great!

icons are quite clear to me, also positioning makes sense. Zoom in makes more sense to be than Double the timespan ... but i agree we could come up with a better tooltip.

alignment and spacing between elements don't look right to me:

padding on icons (10px) is different that on timepicker button (15px)
timepicker button has line-height defined as 20px (should be 16 as everything else in this toolbar)
icon inside timepicker button sets vertical-align: middle (button does the same which misaligns things)
button has 1px trans border, which misaligns things
there is some additional spacing on the left and right (3px) and i have no idea where it comes from.

Bargs · 2016-11-04T21:07:56Z

I'll echo what @tbragin said, some screenshots and a gif in the description, along with some extra explanation of the details of how this is supposed to behave would be very helpful.

lukasolson · 2016-11-29T19:16:30Z

Replaced by #9253.

### Closes elastic#8546

jgowdyelastic added release_note:enhancement review v5.1.0 v6.0.0 labels Oct 5, 2016

jgowdyelastic force-pushed the add-time-nav-buttons branch from b01b1aa to c591bd7 Compare October 5, 2016 13:07

jgowdyelastic changed the base branch from 5.x to master October 5, 2016 13:09

epixa reviewed Oct 5, 2016

View reviewed changes

thomasneirynck self-assigned this Oct 5, 2016

LeeDr added the tests_needed label Oct 6, 2016

thomasneirynck reviewed Oct 6, 2016

View reviewed changes

epixa removed the review label Oct 8, 2016

cjcenizal reviewed Oct 10, 2016

View reviewed changes

jgowdyelastic force-pushed the add-time-nav-buttons branch from 254c731 to 2f7f251 Compare October 11, 2016 12:28

alt74 added the Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. label Oct 11, 2016

thomasneirynck reviewed Oct 24, 2016

View reviewed changes

src/ui/public/styles/base.less Outdated

Copy link
Copy Markdown

Contributor

thomasneirynck Oct 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for me, I think it looks good.

kimjoar reviewed Oct 24, 2016

View reviewed changes

jgowdyelastic added 4 commits October 24, 2016 11:11

Add time navigation buttons

7e1932f

changing vars to const

ad58aaa

Address some review points

4e976af

fixing typo in function name

efad64d

jgowdyelastic force-pushed the add-time-nav-buttons branch from 2f7f251 to efad64d Compare October 24, 2016 10:23

tbragin added Feature:Timepicker Timepicker :Discovery labels Nov 4, 2016

Bargs self-assigned this Nov 4, 2016

ppisljar removed the v5.1.0 label Nov 21, 2016

Bargs assigned lukasolson and unassigned Bargs Nov 22, 2016

lukasolson mentioned this pull request Nov 29, 2016

Add time navigation buttons #9253

Merged

lukasolson closed this Nov 29, 2016

jimgoodwin added the v6.0.0-rc1 label Sep 28, 2017

daveyholler added a commit to daveyholler/kibana that referenced this pull request Nov 7, 2024

Removes the extra "connector" from the API key name

f9a5e75

### Closes elastic#8546

Conversation

jgowdyelastic commented Oct 5, 2016 • edited by epixa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

epixa commented Oct 5, 2016

Uh oh!

epixa commented Oct 5, 2016

Uh oh!

jgowdyelastic commented Oct 5, 2016

Uh oh!

epixa commented Oct 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeeDr commented Oct 6, 2016

Uh oh!

dimitris-athanasiou commented Oct 6, 2016

Uh oh!

thomasneirynck left a comment

Choose a reason for hiding this comment

Uh oh!

thomasneirynck Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjcenizal Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjcenizal Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbragin commented Oct 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasneirynck commented Oct 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimjoar commented Oct 24, 2016

Uh oh!

ppisljar commented Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bargs commented Nov 4, 2016

Uh oh!

lukasolson commented Nov 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

jgowdyelastic commented Oct 5, 2016 •

edited by epixa

Loading

thomasneirynck Oct 6, 2016 •

edited

Loading

cjcenizal Oct 10, 2016 •

edited

Loading

cjcenizal Oct 10, 2016 •

edited

Loading

ppisljar commented Oct 24, 2016 •

edited

Loading